-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace tweak_buttons_func with an event instead #11777
Replace tweak_buttons_func with an event instead #11777
Conversation
… wasn't due yet, call function on parent widget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sane enough to me.
A downside of this approach is that when you click an item in the vocabulary builder window, it opens a new popup which fires off an event which then potentially needs to be consumed by multiple entries in that vocab window before it actually gets to the entry that was clicked on, while before you immediately know which entry was clicked on. Also pinging @weijiuqiao for feedback on the PR in general |
Something weird is still going on, when closing the vocabbuilder widget it closes fine, but the items displayed in it seem to persist still, if you click in the correct place it will pop up the dictionary item for it. It's as if the vocabbuilder widget was visually closed but is actually still there |
I'm fine with this PR (I have just looked at the frontend part, which it makes cleaner, so nothing against - haven't looked at the plugin, not familiar with it). Just mentionning we somehow have hooks to allow plugin to plug/tweak (may be only plug ? less flexible than getting the button to find the right place like vocabbuilder does), which are both quite similar: (not used by any plugin, although I think I remember we made all that thinking about usage by plugins:) koreader/frontend/apps/reader/modules/readerhighlight.lua Lines 1060 to 1070 in 8211208
(used by the wallabag plugin:) koreader/frontend/apps/reader/modules/readerlink.lua Lines 1496 to 1504 in 8211208
So, you may give it some thoughts if what you'd like to achieve and the vocab plugin could use something similar instead - just for the sake of API consistency.
Feels like these 2 snippets I mentionned are what you'd feel like not very clean ? :) |
I was going to reply I don't see that as problematic (because in my view it's not "plugin state" but "a button that should be there") but I'd forgotten about this particular function. Copying it would indeed be better if it fits this use case. |
I'm fine with these deletions :) |
That's... concerning ^^. The smelly part might be those I'm not familiar with the plugin or what it's trying to do, but for that sort of stuff, we usually target the specific widget we want to close and call its actual usual close method (which is often, granted, |
It doesn't seem to be related to that, replacing them doesn't fix the issue at least. I spent way too long messing with it and it just doesn't wanna work, idk what I'm missing 😅 I ended up working around it instead |
Apologies for being late. |
Oops, I tried looking up words from within the definition popup and my emulator freezes. It's alright when not in the plugin. I'm not sure how it would behave on a real device though. |
…ing with nested lookups
…dow we're trying to close
Damn, that's what I get for trying to be smart with that iter function, it's was getting stuck in an infinite loop
But you already do have a reference to it right? that's the |
Yup, that's exactly how I would have proposed doing that. Small (and opinionated, so feel free to ignore) style nit: I tend to use |
But to expand on my earlier "Close" is weird rant, while what you did would be perfect for a "sane"¹, modern widget, here, you'll want to explicitly call That's because the event that [1]. Ideally, all widget teardown would be stuffed in the |
Is that how it's usually done? I wasn't really a fan of the
I'm still a bit confused. Looking at where that 'Close' event is actually being generated it seems to be specifically for when KOReader gets completely turned off. Is that really what I wanna use here? I would think that it's fine if it just closes the widget, there can still be state remaining since I might (and often do) show the widget again at a later point. The problem I was having here is that the widget didn't actually seem to be closed properly at all, I didn't see it anymore but I could still interact with it, like it was still there. Also, I don't know what that TitleBar:onClose is supposed to be doing exactly, I don't see where that function is actually defined, I'm not seeing it in TitleBar itself nor in any of its super ''class'' widgets. |
For my own understanding, let's say I have a plugin. This plugin itself is a
Sorry for all the questions, but whenever I try to interact with any UI related stuff I often lose a LOT of time just trying to get it to work, and usually IDK what I'm actually doing wrong, I would like to understand this stuff properly 😄 |
Yep.
No, which was exactly my point ;).
I probably skipped some vital context in my earlier message ;D. I was looking at how DictQUickLookup itself was handling its own "close my things" codepaths, because as I hinted in my rants, it can be sort of... weird and very much not unified. Which, okay, might look like that's what would happened if you sent a |
There's two sides to this question:
The whole thing means what you add or don't add to the WidgetContainer array sort or depends on whether they need to receive Events or not. IIRC, we generally don't bother to make a difference, and just chuck the whole tree in there.
The whole propagation system means that you generally show/close your top-level WidgetContainer, and then only handle those events where relevant (most likely also just the top-level container ;p. Assuming you don't have sticky pinned references straying in something that is not an instance member (e.g., storing refs in the module and not the instance), destroying the top-level widget container should lead to the whole tree being marked for garbage collection). |
…ion now that event propagation issue seems to be gone
Aha, makes sense now, I'll change it to onClose then. But in general it's still best to call UIManager:close if I wanna close a visible widget?
Why should it be implemented for these? Thanks a lot for the write-up btw, that cleared things up for me. I went ahead and replace
Is this another legacy thing? 😄 While doing that I also figured out what the issue was here #11777 (comment) I guess I was assuming that closing a widget did more than it actually does. Closing So with that problem solved I could go back to my initial implementation, which actually works properly now |
Yep!
Ideally, yes. In practice, rarely, unless you're actually working from within the widget in question itself.
insert whilhelm scream here :D. See my first answer? :D.
Because you can only
There's a reason for the nested arrays. I can't recall what it is exactly, but it makes sense, I think :D. EDIT: Okay, yeah, it's to differentiate between exact matches and "match anything found in this table". See InputContainer's docs. |
(Which, incidentally, |
Dammit, I figured it'd be okay as long as I keep the onClose() one only for DictQuickLookup, it seems to work fine 😅 I'll change it back for all these 'building block' widgets then and only keep it for widgets created by the vocabbuilder plugin itself.
Makes sense, but can't you close things just fine with the
I'm not talking about the nested array, but about the fact it's using |
Ah nice, yeah it seems like it uses it already but only on init, not on destruction when I actually need it now |
Hmm... but that's actually all the stuff I replaced, it's all for container widgets created in the plugin code itself, so it should be safe to use UIManager:close instead of self:onClose(), no? |
Oh, right, mapping the "Back" group of keys to So, yeah, you do need an implementation for those cases; and again: confusing, definitely ^^.
I stopped checking after a while, but you'll notice that all of these classes happen to do extra things in their So you'd be skipping all that stuff by using
Yeah, CloseWidget is sent by |
…closed, meaning we are now consuming Close events which are meant to go to the reader/filemanager
Indeed, that's why I moved that stuff to closeWidget instead, so it would still be called. Because of that I think it'd be safe to do. However, since I need to keep those onClose() calls around anyway so the key bindings still work, I might as well not touch that stuff at all, it is kinda out of scope here anyway. I did notice another small thing still, because the |
Yeah, that's... weird. I'm unfamiliar with this widget in particular, but a widget instance really ought to be gone after a close. |
Eeh, because it's recycled by |
… when closing the window Some of the callback stuff can just sit in the VocabularyBuilderWidget directly too so change that as well
Ok, I gave it yet another go, making sure that VocabularyBuilderWidget is gone once we actually close it, I also moved some of that callback stuff around because it wasn't that easy to read (imo) and it felt like it belonged in that VocabularyBuilderWidget 'class' directly instead of being inserted on init as callbacks Tested it a bunch and it all seems okay still. |
Haven't looked at it too deeply (and am not familiar with it to begin with), but at a glance that looks more like what I would have expected something like that to look like, so, +1 ;). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with the core changes.
(Did not review the plugin parts, not familiar with it.)
There is nothing really wrong with the current approach, considering it was added for one specific reason, giving the vocabbuilder plugin access to the popup dictionary.
I created a custom plugin which also adds a button to the popup dictionary, and up until now I didn't care about it overwriting what the vocabbuilder one does.
However, I got a request last week for it not to do that 🙂
I tried getting around making changes to koreader itself by wrapping the
tweak_buttons_func
so it still calls the original one, but also my own on top of it. This didn't work out quite right because each time the user switches between reader and filemanager all plugins get reloaded, meaning it forgets about the function already being wrapped (since DictQuickLookup itself persists the whole time), and it wraps it again, meaning I end up with one extra button on each reload.On top of that, vocabbuilder also sets this function to nil again at some points (when the setting to automatically add all words looked up is turned on) which I can't detect at all currently
@NiLuJe suggested to make tweak_buttons_func a table instead where each plugin which wants to modify it can store their function in, that still didn't feel very clean to me, and it would mean we're still storing plugin state in the actual DictQuickLookup module directly itself which doesn't seem great to begin with.
I ended up creating a customt even instead, which plugins that care about which buttons appear in the popup dict can consume, it takes in the popup window itself, and the list of buttons. This list is modified in-place.
The upside of this is that you don't need to pass that tweak_buttons_func around anywhere in ReaderDictionary and no plugin state is stored in the persisting DictQuickLookup module
If people have suggestions for better ways to do this I would love to hear it. It would also be great if someone more familiar with vocabbuilder looks at it. I tested it and it seems to work fine but I'm not an active user so I might have missed something.
FYI this was discussed on Gitter first
This change is